Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix long_free_dirty accounting for small files #16264

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Fix long_free_dirty accounting for small files #16264

merged 1 commit into from
Jul 23, 2024

Conversation

tuxoko
Copy link
Contributor

@tuxoko tuxoko commented Jun 14, 2024

Motivation and Context

Description

For files smaller than recordsize, it's most likely that they don't have L1 blocks. However, current calculation will always return at least 1 L1 block.

In this change, we check dnode level to figure out if it has L1 blocks or not, and return 0 if it doesn't. This will reduce the chance of unnecessary throttling when deleting a large number of small files.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

amotin
amotin reviewed Jun 14, 2024
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems OK, but I think we could simplify it even more by checking dn->dn_nlevels <= 1 before doing any math at all, and just returning *start = minimum; and *l1blks = 0;.

@tuxoko
Copy link
Contributor Author

tuxoko commented Jun 14, 2024 •
edited
Loading

It seems OK, but I think we could simplify it even more by checking dn->dn_nlevels <= 1 before doing any math at all, and just returning *start = minimum; and *l1blks = 0;.

I guess so. I'll change it then.

@davidchenntnx
Fix long_free_dirty accounting for small files
4b56f52 
For files smaller than recordsize, it's most likely that they don't have
L1 blocks. However, current calculation will always return at least 1 L1
block.

In this change, we check dnode level to figure out if it has L1 blocks
or not, and return 0 if it doesn't. This will reduce the chance of
unnecessary throttling when deleting a large number of small files.

Signed-off-by: Chunwei Chen <david.chen@nutanix.com>
@tuxoko tuxoko force-pushed the long_free_dirty branch from 154a8b3 to 4b56f52 Compare June 14, 2024 18:51
amotin
amotin approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just put it after the next comment before total_l1blks, since it is a special case of it.

@satmandu satmandu mentioned this pull request Jul 19, 2024
Merged
13 tasks
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jul 23, 2024
@tonyhutter tonyhutter merged commit 9dfc5c4 into openzfs:master Jul 23, 2024
22 of 25 checks passed
tonyhutter pushed a commit that referenced this pull request Aug 6, 2024
@tuxoko @davidchenntnx @tonyhutter
Fix long_free_dirty accounting for small files (#16264)
ef08cb2 
For files smaller than recordsize, it's most likely that they don't have
L1 blocks. However, current calculation will always return at least 1 L1
block.

In this change, we check dnode level to figure out if it has L1 blocks
or not, and return 0 if it doesn't. This will reduce the chance of
unnecessary throttling when deleting a large number of small files.

Signed-off-by: Chunwei Chen <david.chen@nutanix.com>
Co-authored-by: Chunwei Chen <david.chen@nutanix.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
@tuxoko @davidchenntnx @lundman
Fix long_free_dirty accounting for small files (openzfs#16264)
18bd275 
For files smaller than recordsize, it's most likely that they don't have
L1 blocks. However, current calculation will always return at least 1 L1
block.

In this change, we check dnode level to figure out if it has L1 blocks
or not, and return 0 if it doesn't. This will reduce the chance of
unnecessary throttling when deleting a large number of small files.

Signed-off-by: Chunwei Chen <david.chen@nutanix.com>
Co-authored-by: Chunwei Chen <david.chen@nutanix.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amotin amotin amotin approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tuxoko @behlendorf @amotin @tonyhutter @davidchenntnx